Cover art UX improvements #1207

Be-ing · 2017-03-06T04:05:44Z

Rework the UX for showing DlgCoverArtFullSize:

don't close DlgCoverArtFullSize when moving cursor off WCoverArt. IMO this was surprising and confusing.
close DlgCoverArtFullSize when clicked
show DlgCoverArtFullSize when right clicking WSpinny. Before, right clicking on WSpinny paused the track and enabled slip mode. I can't think of a use case for that behavior. The change allows skins to show only WSpinny with cover art but without taking space with a redundant WCoverArt and still make DlgCoverArtFullSize available.

This will allow skins to make the DlgCoverArtFullSize feature available without needing space for both WCoverArt and WSpinny

Be-ing · 2017-03-06T07:26:49Z

src/widget/wspinny.cpp

+            // Trigger a mouse move to immediately line up the vinyl with the cursor
+            mouseMoveEvent(e);
+        }
+    } else if (m_bShowCover) {


Note that this will also work for middle clicks.

daschuer · 2017-03-06T21:26:18Z

Ah Ok. The first two commits are Ok. I am unsure with the last.

In case of a dedicated cover, the big cover is shown on the left click and a context menu appears on the right click. This is more natural to me but does not fit for the shinny.
Showing the big cover on a control is IMHO unexpected as well.
Can't we add a visual click handler or something to the shinny to allow the cover features?

Be-ing · 2017-03-06T21:39:19Z

I agree that right clicking the spinny is not an intuitive way to show the big cover art. However, this is not an important feature, so I don't think it's a big deal if it's not very easy to discover.

Can't we add a visual click handler or something to the shinny to allow the cover features?

What do you mean?

daschuer · 2017-03-07T07:34:31Z

Something like a hamburger button, that appears when hovering on the cover art spinny. Clicks on it can be handled like other clicks on a cover.

Be-ing · 2017-03-07T14:43:12Z

I think that would feel cluttered and get annoying for users who want to left click the spinny.

daschuer · 2017-03-22T21:27:27Z

Can we come to a conclusion here? Merge as it is?
Can anyone else have a brief look on it?

Be-ing · 2017-03-23T00:48:26Z

I'm in favor of merging as-is, but it would be good for someone else to test.

esbrandt · 2017-03-25T12:53:25Z

don't close DlgCoverArtFullSize when moving cursor off WCoverArt. IMO this was surprising and confusing.

IIRC this was a design decision to avoid some issues, can´t remember the details, see pull #278

Tested your PR on OSX 10.11.1 with latest master:

You can stack multiple FullSize covers, by right clicking on different spinnies, or the library cover art. The FullSize covers are centered, not movable and intransparent, what are the reasons to allow more than one FullSize cover at a time?
The FullSize covers stay on top, even if you open the preferences, or switch programs and Mixxx is in the background. If Mixxx is in the background you can't close the cover art by pressing ESC.
When you change the cover art by using the WCoverArt right-click menu while having the cover on full size display, the full size display does not change to the new cover.
spinny tooltip needs update.

show DlgCoverArtFullSize when right clicking WSpinny. Before, right clicking on WSpinny paused the track and enabled slip mode. I can't think of a use case for that behavior. ...

I find it odd that we have left click for FullSize display on WCoverArt for library and decks, and right-click for spinnies. Although the and left-/ and right- click behavior of WCoverArt is not that obvious though.

... The change allows skins to show only WSpinny with cover art but without taking space with a redundant WCoverArt and still make DlgCoverArtFullSize available.

Removing WCoverArt in the decks as redundant, cuts out the users who wish not to use spinnies for whatever reasons.

Be-ing · 2017-03-25T18:49:07Z

IIRC this was a design decision to avoid some issues, can´t remember the details, see pull #278

I didn't find any discussion about it when briefly looking through that PR, but maybe I didn't look hard enough.

what are the reasons to allow more than one FullSize cover at a time?

I hadn't thought anyone would try that. I'm not sure why you'd want to see another cover without closing the previous one first. Is this really an issue? Even if a user does this, they can just click on each cover to hide it.

Removing WCoverArt in the decks as redundant, cuts out the users who wish not to use spinnies for whatever reasons.

LateNight does not have both and I haven't seen a single user ask or complain about it. On the other hand, the multitude of options related to cover art in Deere 2.0 is confusing. Furthermore, screen space is precious; we should not have redundant widgets using more than they need.

I'll look into the other issues.

esbrandt · 2017-03-25T20:10:25Z

I hadn't thought anyone would try that

Just acting like a potential user. This is a minor issue, otoh your PR aims to fix an minor issue as well.

LateNight does not have both and I haven't seen a single user ask or complain about it.

We introduced Coverart in 2.0. There are no metrics how many of our users change from the default skin (LateNight as of 2.0). LateNight supports coverart in the library (Fullsize), and in the spinnies (underlay image). Those who never change from the default skin, likely not even know other coverart options exist. Also because our 2.0 documentation is incomplete/out-of-date.

Users of Deere/Shade currently have coverart in the decks enabled/spinnies disabled per default. Taking away features in a minor revision we introduced just a major revision earlier - without need, why?

Be-ing · 2017-03-25T20:28:11Z

My primary goal with this PR is to make it possible to show the cover art in only one place (the spinny) and keep the ability to show the big cover without having a redundant cover art widget taking up space. Come to think of it, that does not necessitate removing an option for the cover art widget though.

MK-42 · 2017-03-26T10:26:18Z

I'm not sure why you'd want to see another cover without closing the previous one first. Is this really an issue?

From my point of view it would make most sense to always show only one fullsize cover. Stacking multiple of them feels odd. Can we close the old when opening the next?

Currently clicking on the same small cover-art twice opens and closes the fullsize one again. That makes sense. But I would expect, that every small cover-art click would close any existing, and if the clicked-on is different from the one that was open, the new one should be opened in full size. So always only one big cover-art visible.

* let DlgCoverArtFullSize have a border * set DlgCoverArtFullSize window title to "Album Artist - Album (Year)" (or Album instead of Album Artist if there is no Album Artist) * let DlgCoverArtFullSize be resized either by dragging the window border or using a mouse scroll wheel * update DlgCoverArtFullSize when cover art is changed * update DlgCoverArtFullSize when track is reloaded * show WCoverArtMenu on right click of DlgCoverArtFullSize * show WCoverArtMenu on right click of WSpinny when the track does not have cover art set * update WSpinny tooltip * don't hide DlgCoverArtFullSize when moving mouse off of WCoverArtLabel

Be-ing · 2017-03-28T19:14:56Z

I made DlgCoverArtFullSize a normal window with a border. I think this obviates making only one DlgCoverArtFullSize show at a time, which would be quite complicated to implement. It can now be resized by dragging the window border or by using a mouse scroll wheel. Unfortunately Qt does not make it easy to preserve the aspect ratio of a widget, so the graphic is scaled proportional to its aspect ratio, but there can be blank space on the sides of the window if it is stretched outside of the cover's aspect ratio. Also, right clicking on DlgCoverArtFullSize brings up the cover art context menu. Right clicking on a WSpinny with a track loaded that does not have cover art set also brings up the cover art context menu. DlgCoverArtFullSize responds to changing the cover art and loading new tracks too.

Be-ing · 2017-03-30T00:24:15Z

@esbrandt, could you test again?

MK-42 · 2017-03-30T09:13:57Z

Giving those windows the decorator makes closing them much more obvious, the resize on scroll is also nice.

Before merge the qDebugs should be removed, as the resize event is spamming the log atm. Other than that, 👍

esbrandt · 2017-03-30T19:12:17Z

Looks pretty good now.
Might be some platform specific issue (macOS here) left:

With the latest changes the full size cover still does not respond to cover changes. Cover art in library, and on spinny work as expected.
Right- clicking on the spinny without cover (no matter the deck), brings up the context menu in the upper left corner of the screen (main widget?), not below the cursor.
A track with incomplete metadata (e.g. no artist, but title), brings up an empty window title for the full size cover. What if we just display Mixxxinstead?

Be-ing · 2017-03-30T19:15:25Z

With the latest changes the full size cover still does not respond to cover changes. Cover art in library, and on spinny work as expected.

How are you changing the cover? From the popup window, the spinny, or the library?

Be-ing · 2017-03-30T19:16:23Z

I'm thinking of removing the click to close action now that there is a normal window border. Thoughts?

esbrandt · 2017-03-30T19:20:27Z

I'm thinking of removing the click to close action now that there is a normal window border. Thoughts?

+1 , and we have still ESC to close the window.

esbrandt · 2017-03-30T19:21:46Z

How are you changing the cover? From the popup window, the spinny, or the library?

Library

ronso0 · 2017-04-01T16:51:49Z

Joining this conversation a bit late, I'm afraid I'm the party pooper when I say I don't like this new UX. Sure, that Cover would close when leaving CoverArt icon with mouse, was a bit unexpected but I got used to it. Furthermore, this saves a click when you'd want to quickly get back to skin.

previous, borderless Cover was really elegant, windowed it feels like being thrown back to (in my case, XFCE) ugly OS window styling
Covers taller than screen don't get resized but are cropped by screen and cover the rest of mixxx
Covers also wider than screen get resized to screen width (height cropped), but then I can't scroll to zoom out, I'd have to undock window first

Those aspects I consider a UX and aesthetic regression, there's much more interaction necessary to just view the whole Cover.

Wouldn't your

primary goal with this PR is to make it possible to show the cover art in only one place (the spinny) and keep the ability to show the big cover without having a redundant cover art widget taking up space.

and just removing closing the Cover on mouse-out be enough?

Be-ing · 2017-04-02T02:08:26Z

I'll look into scaling down covers that are larger than the screen. Previously it checked whether the cover was bigger than the largest window of Mixxx, but I don't think that's a great approach, especially with the cover being a normal window with a border. I'll look into how to detect the size of the screen rather than the size of the main Mixxx window.

just removing closing the Cover on mouse-out be enough?

IMO yes, that was enough, but @esbrandt was not satisfied because of the aforementioned issues.

ronso0 · 2017-04-02T02:54:00Z

I'll look into scaling down covers that are larger than the screen.

nice! If cover art is scaled to fit (plus a little space around?) when opened, can be zoomed with scroll wheel and closes on click, there's no need for window decoration anymore, right? Or is this an unavoidable consequence of the new implementation?

Be-ing · 2017-05-12T23:08:48Z

One small question: when scrolling to zoom, it acts like dragging bottom right corner to resize (upper left corner is fixed). Is this window manager dependent or could you make it zoom from/to center?

Good idea. I implemented that.

@esbrandt, could you test this again?

sblaisot · 2017-05-13T11:11:37Z

I would instead prefer zoom from mouse position, that's imho a more natural approach (like in google maps for example).

ronso0 · 2017-05-13T13:37:24Z

I would instead prefer zoom from mouse position, that's imho a more natural approach (like in google maps for example).

I agree, this would be consistent with most graphics applications.

Be-ing · 2017-05-13T15:48:55Z

Good idea @sblaisot. I'll work on that.

with the mouse scroll wheel

Be-ing · 2017-06-01T20:33:12Z

Using the mouse wheel now zooms in to/out from the cursor position. Ready for merge?

daschuer · 2017-06-02T21:38:18Z

The scroll resizing should stop at maximum image size. Else you get white border, which will persist after shrinking again.

daschuer · 2017-06-02T21:42:47Z

The same happens if you shrink to minimum, using a non square cover.

Be-ing · 2017-11-09T04:12:18Z

Admittedly this branch isn't perfect with the issue of whitespace appearing when resizing to extremes, but I think it's a good step forward and good enough for merge as is.

esbrandt · 2017-11-18T10:20:13Z

Admittedly this branch isn't perfect with the issue of whitespace appearing when resizing to extremes, but I think it's a good step forward and good enough for merge as is.

Mostly agree. Resizing theCover library column isn't perfect too when resizing to extremes. We had a discussion back than that it only matters in rare cases, and nonetheless merged the feature .

My only regret with this PR is that we loose the ability to display the Spinnies WITHOUT covers.
I still think we should not force remove the feature.

Be-ing · 2017-11-19T20:23:09Z

My only regret with this PR is that we loose the ability to display the Spinnies WITHOUT covers.
I still think we should not force remove the feature.

This PR does not remove that ability. That was removed from the Deere skin in #940. Adding it back requires changing the skin.

Be-ing · 2017-11-24T18:06:15Z

@esbrandt ready for merge then?

uklotzde · 2017-11-24T18:39:07Z

LGTM 👍 I like it. It really improves the usability of the full size cover art window.

I can deal with the remaining open issues. After upgrading to Qt5 we need to revisit all UI elements again, so I would not spend more time on polishing every detail.

Some final thoughts from the other reviewers? Unfortunately I'm using the same OS and skin as @Be-ing so my opinion might be biased ;)

uklotzde · 2017-11-25T08:46:50Z

src/library/dlgcoverartfullsize.cpp

+            // Scale down dialog to screen size if the pixmap is larger than the screen
+            QSize dialogSize = m_pixmap.size();
+            const QSize availableScreenSpace =
+                QApplication::desktop()->availableGeometry().size();


This is too large in full screen mode. The cover art window extends below the bottom of the screen, because it will always be placed underneath the menu bar of the main window and is not able to move above it. This restriction does not apply if the application uses a regular window. @Be-ing Any ideas for an easy fix?

What window manager do you use? With KWin (X11), I have a different issue with full screen mode. In that case, the cover art window behaves as if my bottom menu bar is still showing as if I was not in full screen mode.

We seem to have different issues with different software setups and these very well could change with Qt5, so I am doubtful it is worthwhile to perfect all the edge cases right now.

Using availableGeometry().size() * 0.9 instead of the desktop size keeps the main window visible around the cover art popup in all directions and works much better for me. Simple but effective.

Clever workaround!

daschuer · 2017-11-26T22:06:54Z

The general behaviour is OK.
There is an issue that the cover window moves around depending on the size of the previous cover. I think it should either save the last window position or display always centred.
Currently only the second window call up is perfectly centred.
This is nothing that prevent merging though, so LGTM.

I don't know why the resizeEvent triggered by setGeometry does not take care of this, but hey, this works.

Be-ing · 2017-11-30T04:45:21Z

There is an issue that the cover window moves around depending on the size of the previous cover. I think it should either save the last window position or display always centred.

Fixed. The window is centered when loading a new cover art image now. I think this PR is ready for merge.

uklotzde · 2017-11-30T19:36:44Z

LGTM. Thanks for your patience, Be ;)

ronso0 · 2017-12-12T15:58:43Z

Just found a bug:
When I zoom into a cover art window so that it's bigger than the screen and then close it, I can't zoom out when I open it again. Manual resizing works though.

Be-ing added 3 commits March 5, 2017 22:00

don't close big cover art dialog when moving mouse cursor off WCoverArt

2eb2800

close DlgCoverArtFullSize when clicked

6226e14

show DlgCoverArtFullSize on spinny right click

03557a7

This will allow skins to make the DlgCoverArtFullSize feature available without needing space for both WCoverArt and WSpinny

Be-ing commented Mar 6, 2017

View reviewed changes

This was referenced Mar 6, 2017

Tango skin #1151

Merged

Deere 2.1 #940

Merged

explicitly compare to nullptr

8fe523e

Be-ing added 2 commits May 12, 2017 18:06

avoid log spam

ad97f72

keep DlgCoverArtFullSize's center with mouse scroll resizing

099ddb2

comment out debugging messages

4d89c7c

keep point under cursor when resizing DlgCoverArtFullSize

aed9738

with the mouse scroll wheel

uklotzde added the ui label Oct 29, 2017

uklotzde reviewed Nov 25, 2017

View reviewed changes

uklotzde added this to the 2.1.0 milestone Nov 25, 2017

Be-ing added 2 commits November 29, 2017 15:45

work around for whitespace appearing when resizing cover art window

263a154

fix centering of DlgCoverArtFullSize when loading new cover

4c4d8e7

Be-ing force-pushed the cover_art_ux branch from 76f4b11 to 4c4d8e7 Compare November 30, 2017 03:43

DlgCoverArtFullSize: resize newly loaded cover art to fit window

3adb50a

I don't know why the resizeEvent triggered by setGeometry does not take care of this, but hey, this works.

uklotzde merged commit 279236f into mixxxdj:master Nov 30, 2017

Be-ing deleted the cover_art_ux branch November 30, 2017 20:08

mixxxbot mentioned this pull request Aug 22, 2022

slip mode wrong behavior if deck is playing a Loop #6993

Closed

Cover art UX improvements #1207

Cover art UX improvements #1207

Conversation

Be-ing commented Mar 6, 2017 • edited Loading

Be-ing Mar 6, 2017

Choose a reason for hiding this comment

daschuer commented Mar 6, 2017

Be-ing commented Mar 6, 2017

daschuer commented Mar 7, 2017

Be-ing commented Mar 7, 2017

daschuer commented Mar 22, 2017

Be-ing commented Mar 23, 2017

esbrandt commented Mar 25, 2017

Be-ing commented Mar 25, 2017

esbrandt commented Mar 25, 2017

Be-ing commented Mar 25, 2017

MK-42 commented Mar 26, 2017

Be-ing commented Mar 28, 2017

Be-ing commented Mar 30, 2017

MK-42 commented Mar 30, 2017

esbrandt commented Mar 30, 2017

Be-ing commented Mar 30, 2017

Be-ing commented Mar 30, 2017

esbrandt commented Mar 30, 2017

esbrandt commented Mar 30, 2017

ronso0 commented Apr 1, 2017

Be-ing commented Apr 2, 2017

ronso0 commented Apr 2, 2017

Be-ing commented May 12, 2017

sblaisot commented May 13, 2017

ronso0 commented May 13, 2017

Be-ing commented May 13, 2017

Be-ing commented Jun 1, 2017

daschuer commented Jun 2, 2017

daschuer commented Jun 2, 2017

Be-ing commented Nov 9, 2017

esbrandt commented Nov 18, 2017

Be-ing commented Nov 19, 2017

Be-ing commented Nov 24, 2017

uklotzde commented Nov 24, 2017

uklotzde Nov 25, 2017

Choose a reason for hiding this comment

Be-ing Nov 27, 2017

Choose a reason for hiding this comment

Be-ing Nov 27, 2017

Choose a reason for hiding this comment

uklotzde Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Be-ing Nov 29, 2017

Choose a reason for hiding this comment

daschuer commented Nov 26, 2017

Be-ing commented Nov 30, 2017

uklotzde commented Nov 30, 2017

ronso0 commented Dec 12, 2017

Be-ing commented Mar 6, 2017 •

edited

Loading

uklotzde Nov 27, 2017 •

edited

Loading